Skip to content

Replace custom OpenPGP packet parsing with pysequoia #7433

Open
dralley wants to merge 2 commits intopulp:mainfrom
dralley:use-pysequoia
Open

Replace custom OpenPGP packet parsing with pysequoia #7433
dralley wants to merge 2 commits intopulp:mainfrom
dralley:use-pysequoia

Conversation

@dralley
Copy link
Copy Markdown
Contributor

@dralley dralley commented Mar 9, 2026

📜 Checklist

  • Commits are cleanly separated with meaningful messages (simple features and bug fixes should be squashed to one commit)
  • A changelog entry or entries has been added for any significant changes
  • Follows the Pulp policy on AI Usage
  • (For new features) - User documentation and test coverage has been added

See: Pull Request Walkthrough

@dralley
Copy link
Copy Markdown
Contributor Author

dralley commented Mar 9, 2026

@mdellweg @gerrod3 This is first draft quality, I have not thoroughly reviewed this. But let me know what you think, and feel free to look at the pysequoia PR as well.

I will note, that I don't think pysequoia actually supports PQC yet (the patches exist but aren't merged yet, I think RH is applying them downstream in places). So we will need to make sure that it's not doing "too much" validation

Requires wiktor-k/pysequoia#61

@dralley dralley force-pushed the use-pysequoia branch 4 times, most recently from a4007d3 to ccf9278 Compare March 10, 2026 01:36
@dralley dralley changed the title Use pysequoia Replace custom OpenPGP packet parsing with pysequoia Mar 10, 2026
username (str): Empty string; primary UID not available without extending pysequoia.
data (bytes or None): The verified plaintext content for inline signatures, None for
detached signatures.
"""
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not actually sure that we need this, but gpg_verify is public API, so I figured we would have to at least try. Unless we wait until the next breaking change release.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pulp_ansible seems to use the result in a trivial way, we could delete most of this at the next breaking change release.

Copy link
Copy Markdown
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.

"raw_data": body,
"created": packet.signature_created,
}
# Note: Pulp's concept of "expiration time" is a duration starting at creation time
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh it should have been expiration date?
"time" really is ambiguous.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBF, pulp is using the terminology that was defined in the spec, so it was "correct", it's just that the terminology in the spec was ambiguous.

https://www.rfc-editor.org/rfc/rfc9580.html#name-signature-expiration-time

Opinions probably differ whether Sequoia providing expiration_time as something else is a good idea. Less ambiguous wording, but a bit confusing when it comes to the spec.

@dralley dralley force-pushed the use-pysequoia branch 3 times, most recently from 56bd4a3 to 4441274 Compare March 11, 2026 15:16
@dralley dralley force-pushed the use-pysequoia branch 2 times, most recently from 38ee7d1 to a234584 Compare April 4, 2026 17:22
@dralley dralley force-pushed the use-pysequoia branch 2 times, most recently from e232d0d to 9827fcc Compare April 4, 2026 17:46
@dralley dralley marked this pull request as ready for review April 4, 2026 18:43
@dralley dralley requested a review from mdellweg April 4, 2026 18:43
pytest-custom_exit_code
pytest-xdist
python-gnupg
pysequoia
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we even repeating the dependency here? (It's not optional in pyproject, right?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

functional tests can run outside the environment where Pulp is installed.

Comment on lines -1160 to +1173
return gpg, fingerprint, keyid
return cert, fingerprint, keyid
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is anything outside of pulpcore using this fixture?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like yes https://github.com/search?q=org%3Apulp%20signing_gpg_metadata&type=code

pulp_container and pulp_deb at least, pulp_rpm looks like it might have a copy of an identical name

keyid = fingerprint[-16:]

gpg.trust_keys(fingerprint, "TRUST_ULTIMATE")
gpg_cmd = ["gpg", "--homedir", str(signing_gpg_homedir_path)]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this now using sequoia too, depending on the actual binary installed?

Copy link
Copy Markdown
Contributor Author

@dralley dralley Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's using whichever utility is behind the command named gpg, which AFAIK is still just gpg. I don't think RHEL 10 is using a replacement under the gpg name

self.pubkey_fingerprint = vs.certificate.upper()
self.key_id = vs.signing_key[-16:].upper()
self.username = ""
else:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this branch mean? Can a signature be valid when there are no signatures at all?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you are correct, it's dead code. Removed

)

gpg = gnupg.GPG(gnupghome=options["gnupghome"], keyring=options["keyring"])
gpg_cmd = ["gpg"]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we need this to use sequoia too in order to handle all the new algorithms and keyfile formats?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even sequoia doesn't "support" them yet because the IETF draft isn't final yet. You have to install a special build to get them. So it's out of scope for now. At some point, probably, but even then it might be a host-dependent thing.

Use pysequoia instead, or shell out to gpg on the command line, which is
what python-gnupg does anyway. It's not much additional code to just
ditch the dependency everywhere.

Assisted-By: claude-opus-4.6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants